Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't fail with InvalidLength when reading nothing at end of data #1387

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

mpalmer
Copy link
Contributor

@mpalmer mpalmer commented Apr 22, 2024

This is a curious corner case that can happen when, for example, you're reading the length-value encoded strings in OpenSSH. If the last string in the data is of zero-length, then decode() will be called with an empty buffer, so we don't need to read any bytes. However, since is_finished() was checked unconditionally, the zero-length read (which would be OK) was instead rejected because there was no data to (not) read.

The bug was particularly mischievous because is_finished() will return false if there's = padding, so 2/3 of empty read calls will work normally.

mpalmer added a commit to mpalmer/rust-crypto-ssh that referenced this pull request Apr 22, 2024
If a "PEM-like" OpenSSH format private key:

1. has no comment;
2. is of a length such that there is no padding bytes at the end of the "Unencrypted list of N private keys"; ***and***
3. is of a length such that the base64-encoded form of the key does not require any `=` bytes for padding;

then `ssh_key::private::PrivateKey::from_openssh` fails to successfully parse the key.

The root cause is `base64ct` (prior to RustCrypto/formats#1387) incorrectly rejecting zero-length reads when the base64-encoded buffer was empty.
This wouldn't happen if:

1. the key had a comment, because that wouldn't be a zero-length read;
2. there were padding bytes in the "Unencrypted list of N private keys", because those padding bytes would still be in the buffer, so the buffer wasn't empty; *or*
3. there were base64 `=` padding characters, because those would cause the decoder to believe a read would succeed (even though it wouldn't if the read was longer than zero bytes).

Debugging this was... an amusement.

The most reliable way to demonstrate this is with an ECDSA P-256 key without a comment (easily generated with `ssh-keygen -t ecdsa -b 256 -C '' -N '' -f <file>`),
because they *usually* (but not *always*) are of an appropriate length to trigger the bug.
Amusingly (cough), the existing ECDSA P-256 key in `ssh-key/tests/examples/id_ecdsa_p256` is one of the outliers, as its `d` is 33 bytes, rather than 32, which means that key (when stripped of its comment) *doesn't* trigger the bug.
mpalmer added a commit to mpalmer/rust-crypto-ssh that referenced this pull request Apr 22, 2024
If a "PEM-like" OpenSSH format private key:

1. has no comment;
2. is of a length such that there is no padding bytes at the end of the "Unencrypted list of N private keys"; ***and***
3. is of a length such that the base64-encoded form of the key does not require any `=` bytes for padding;

then `ssh_key::private::PrivateKey::from_openssh` fails to successfully parse the key.

The root cause is `base64ct` (prior to RustCrypto/formats#1387) incorrectly rejecting zero-length reads when the base64-encoded buffer was empty.
This wouldn't happen if:

1. the key had a comment, because that wouldn't be a zero-length read;
2. there were padding bytes in the "Unencrypted list of N private keys", because those padding bytes would still be in the buffer, so the buffer wasn't empty; *or*
3. there were base64 `=` padding characters, because those would cause the decoder to believe a read would succeed (even though it wouldn't if the read was longer than zero bytes).

Debugging this was... an amusement.

The most reliable way to demonstrate this is with an ECDSA P-256 key without a comment (easily generated with `ssh-keygen -t ecdsa -b 256 -C '' -N '' -f <file>`),
because they *usually* (but not *always*) are of an appropriate length to trigger the bug.
Amusingly (cough), the existing ECDSA P-256 key in `ssh-key/tests/examples/id_ecdsa_p256` is one of the outliers, as its `d` is 33 bytes, rather than 32, which means that key (when stripped of its comment) *doesn't* trigger the bug.
base64ct/src/decoder.rs Outdated Show resolved Hide resolved
@tarcieri
Copy link
Member

Is this really necessary and the best solution? Wouldn’t it be better not to perform a zero length read in the first place?

@mpalmer
Copy link
Contributor Author

mpalmer commented Apr 22, 2024

Is this really necessary and the best solution? Wouldn’t it be better not to perform a zero length read in the first place?

If "don't perform zero length reads" is the best solution, that's cool, although in that case it would be better, IMO, if this method always rejected zero length reads. The thing that made this bug so hard to track down was the fact that the zero length read only failed in a very narrow set of circumstances.

base64ct/src/decoder.rs Outdated Show resolved Hide resolved
@tarcieri
Copy link
Member

I think it's fine to always reject zero length reads

@mpalmer
Copy link
Contributor Author

mpalmer commented Apr 24, 2024

Righto, I'll rework the PR to go that way.

mpalmer added a commit to mpalmer/rust-crypto-ssh that referenced this pull request Apr 24, 2024
Adjust to the changes to `base64ct` that reject zero-length decode() calls as invalid.

The underlying trigger for this change was to avoid problems with decoding certain OpenSSH private keys.

If a "PEM-like" OpenSSH format private key:

1. has no comment;
2. is of a length such that there is no padding bytes at the end of the "Unencrypted list of N private keys"; ***and***
3. is of a length such that the base64-encoded form of the key does not require any `=` bytes for padding;

then `ssh_key::private::PrivateKey::from_openssh` failed to successfully parse the key.

The root cause was `base64ct` (prior to RustCrypto/formats#1387) incorrectly rejecting zero-length reads, *only* when the base64-encoded buffer was empty.
This only happened for reading empty comments, because comments are at the end of the key.
It *wouldn't* happen if:

1. the key had a comment, because that wouldn't be a zero-length read;
2. there were padding bytes in the "Unencrypted list of N private keys", because those padding bytes would still be in the buffer, so the buffer wasn't empty; *or*
3. there were base64 `=` padding characters, because those would cause the decoder to believe a read would succeed (even though it wouldn't if the read was longer than zero bytes).

Debugging this was... an amusement.

The most reliable way to demonstrate this was with an ECDSA P-256 key without a comment (easily generated with `ssh-keygen -t ecdsa -b 256 -C '' -N '' -f <file>`),
because they're *usually* (but not *always*) an appropriate length to trigger the bug.
Amusingly (cough), the existing ECDSA P-256 test key in `ssh-key/tests/examples/id_ecdsa_p256` is one of the outliers, as its `d` is 33 bytes, rather than 32, which means that key (when stripped of its comment) *doesn't* trigger the bug.
@mpalmer
Copy link
Contributor Author

mpalmer commented Apr 24, 2024

OK, base64ct now errors on zero-length decode requests, with InvalidLength.

tarcieri pushed a commit to RustCrypto/SSH that referenced this pull request Apr 24, 2024
Adjust to the changes to `base64ct` that reject zero-length decode() calls as invalid.

The underlying trigger for this change was to avoid problems with decoding certain OpenSSH private keys.

If a "PEM-like" OpenSSH format private key:

1. has no comment;
2. is of a length such that there is no padding bytes at the end of the "Unencrypted list of N private keys"; ***and***
3. is of a length such that the base64-encoded form of the key does not require any `=` bytes for padding;

then `ssh_key::private::PrivateKey::from_openssh` failed to successfully parse the key.

The root cause was `base64ct` (prior to RustCrypto/formats#1387) incorrectly rejecting zero-length reads, *only* when the base64-encoded buffer was empty.
This only happened for reading empty comments, because comments are at the end of the key.
It *wouldn't* happen if:

1. the key had a comment, because that wouldn't be a zero-length read;
2. there were padding bytes in the "Unencrypted list of N private keys", because those padding bytes would still be in the buffer, so the buffer wasn't empty; *or*
3. there were base64 `=` padding characters, because those would cause the decoder to believe a read would succeed (even though it wouldn't if the read was longer than zero bytes).

Debugging this was... an amusement.

The most reliable way to demonstrate this was with an ECDSA P-256 key without a comment (easily generated with `ssh-keygen -t ecdsa -b 256 -C '' -N '' -f <file>`),
because they're *usually* (but not *always*) an appropriate length to trigger the bug.
Amusingly (cough), the existing ECDSA P-256 test key in `ssh-key/tests/examples/id_ecdsa_p256` is one of the outliers, as its `d` is 33 bytes, rather than 32, which means that key (when stripped of its comment) *doesn't* trigger the bug.
@tarcieri tarcieri merged commit a375cbf into RustCrypto:master Apr 24, 2024
52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants